Skip to content

bpo-34127: Fix grammar in error message with respect to argument count#8395

Merged
rhettinger merged 2 commits into
python:masterfrom
tirkarthi:bpo34127
Jul 22, 2018
Merged

bpo-34127: Fix grammar in error message with respect to argument count#8395
rhettinger merged 2 commits into
python:masterfrom
tirkarthi:bpo34127

Conversation

@tirkarthi

@tirkarthi tirkarthi commented Jul 22, 2018

Copy link
Copy Markdown
Member
  • Modified the error message to be argument or arguments based on argument count.
  • Added tests in test_call.py based on the changes made.
  • Added a NEWS entry since the change will be seen by users.

https://bugs.python.org/issue34127

@terryjreedy

Copy link
Copy Markdown
Member

Raymond, you merged this just before I tried to submit a review suggesting changes and expansion to include all the fixes needed in the file. I cannot submit the review and AFAIK, cannot turn pending review comments into regular comments, so I am copying most of them here before before cancelling the review.

Tests: In the 3 tests before this, the suffix 0, 2, or 3 is the number of arguments given. This would suggest 0a, 0b, 2a, and 3a for the suffixes here. I like 1min, 2min, 1max, and 2max even better. You can wait a bit for other reviews before changing.

Blurb:

Let us be more specific. For the patch as is,
Fix pluralization of 'argument' in TypeError message with 'expected at least/most 1 argument', instead of '1 arguments'.

But if we add all the remaining fixes needed in this file (see below), we could write.
Fix pluralization in TypeError messages in getargs.c: '1 argument' instead of '1 arguments' and '1 element' instead of '1 elements'.

Fixes: This patch follows existing expressions formatted into argument%s:
line 380: (nargs < min ? min : max) == 1 ? "" : "s",
lines 1667 and 2093: (len == 1) ? "" : "s", (parens not needed)

Scope: I think we should finish the job in the file. The next line in both else clauses,
"unpacked tuple should have %s%zd elements,"
should get the same treatment. This may break existing tests. (These are the only two formatted 'elements'.

Fixes for the following will break some test, which will then need fixing also.
542: "expected %d arguments, not %" PY_FORMAT_SIZE_T "d",
1720, "%.200s%s takes %s %d positional arguments"
1799, "%.200s%s takes %s %d positional arguments"
2106, "%200s%s takes %s %d positional arguments (%d given)",
2154: "%.200s%s takes %s %d positional arguments"

I checked all PyErr_Format entries and did not see anything other than %d arguments and %d elements.

@tirkarthi

Copy link
Copy Markdown
Member Author

@terryjreedy Thanks much for the comments. Since the current issue was closed I will open a new one as I get home in a couple of hours with the above comment as a continuation of work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants